Evaluate hashicorp/raft upstream migration#59
Conversation
Detailed analysis of divergence between openark/raft (2017 fork) and upstream hashicorp/raft v1.7+. Catalogs 10 divergent API calls, assesses security risk, estimates migration effort at 3-5 days, and recommends migrating to upstream. Closes #56
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive analysis document to evaluate the migration from an outdated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive evaluation document for migrating from a forked hashicorp/raft to the upstream version. The analysis is thorough and well-structured, providing a strong case for the migration. My review focuses on improving the clarity and accuracy of this new documentation. I've pointed out a few areas for improvement: clarifying the age of the fork to avoid confusion, correcting a technical description of the custom Yield() API, and fixing a structural error where a compatible API was listed under 'Removed APIs'.
|
|
||
| ## Executive Summary | ||
|
|
||
| Orchestrator depends on `openark/raft`, a 2017-era fork of `hashicorp/raft` pinned at commit `fba9f909f7fe` (September 2017). The fork diverges significantly from upstream `hashicorp/raft` v1.7+, which has undergone nine years of active development including security fixes, performance improvements, and major API changes. |
There was a problem hiding this comment.
The document, dated 2026, repeatedly refers to the 2017 fork as being 'nine years' old. While this is consistent with the document's date, it may be confusing for present-day readers. To improve clarity and make the document more timeless, consider phrasing the age relative to the fork date, for example: '...undergone many years of active development since 2017...'. This phrasing is also more robust against the passage of time. This point applies to lines 9, 172, 176, 229, and 262.
| #### `raft.Raft.Yield()` | ||
|
|
||
| - **Location:** `raft.go:284` | ||
| - **What it does:** Causes the leader to voluntarily step down in favor of a specific peer. This is a custom addition in the openark fork, not present in standard hashicorp/raft. |
There was a problem hiding this comment.
The description for raft.Raft.Yield() says it causes the leader to step down 'in favor of a specific peer'. This is slightly inaccurate. The underlying raft.Yield() implementation in the fork performs a non-targeted step-down and temporarily suspends its own leadership candidacy. The FSM logic calling it has a target peer, but only to avoid yielding to itself. Clarifying this distinction would make the comparison with upstream's targeted LeadershipTransferToServer more precise.
| #### `raft.Raft.LeaderCh()` | ||
|
|
||
| - **Location:** `raft.go:154` | ||
| - **What it does:** Returns a channel that signals leadership changes. | ||
| - **Upstream status:** `LeaderCh()` still exists in upstream v1.7.x. | ||
| - **Migration effort:** **None.** API is compatible. |
There was a problem hiding this comment.
The raft.Raft.LeaderCh() API is listed under section '2.1 Removed APIs', but its description correctly states that it is compatible with upstream. This is confusing. To improve clarity, this section should be removed, as the API is already correctly listed as compatible in the table in section 2.2.
There was a problem hiding this comment.
Pull request overview
Adds an evaluation document assessing migration from the github.com/openark/raft fork (via go.mod replace) to upstream github.com/hashicorp/raft v1.7.x, documenting API divergences, risk tradeoffs, and a rough effort estimate to guide future migration work.
Changes:
- Added
docs/raft-evaluation.mdwith a catalog of raft API divergences and proposed migration paths. - Included a security/risk assessment for staying on the 2017 fork versus migrating upstream.
- Provided a task breakdown and suggested migration order.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | `raft.Log` struct | `fsm.go:33`, `rel_store.go:179,196,201` | Compatible | | ||
| | `raft.FSMSnapshot` interface | `fsm.go:84` | Compatible | | ||
| | `raft.SnapshotSink` interface | `fsm_snapshot.go:35` | Compatible | | ||
| | `raft.SnapshotMeta` struct | `file_snapshot.go:55,137,154,191,199,273` | Compatible (but `Peers` field changed to `Configuration`) | |
There was a problem hiding this comment.
In the “Compatible APIs” table, raft.SnapshotMeta is marked as “Compatible” even though the note says the Peers field changes to Configuration upstream. Since this field is used by FileSnapshotStore today, this is a migration-breaking divergence; consider moving this row to the divergent section or changing the status to make it clear code changes are required.
| | `raft.SnapshotMeta` struct | `file_snapshot.go:55,137,154,191,199,273` | Compatible (but `Peers` field changed to `Configuration`) | | |
| | `raft.SnapshotMeta` struct | `file_snapshot.go:55,137,154,191,199,273` | Divergent – `Peers` field replaced by `Configuration`; migration requires code changes in `FileSnapshotStore`. | |
Summary
docs/raft-evaluation.mdwith a detailed analysis of migrating fromopenark/raft(2017 fork) to upstreamhashicorp/raftv1.7+Key findings
SnapshotStore.Create()signature change)RelationalStore(LogStore/StableStore) is fully compatible with upstream -- zero changes neededTest plan
Closes #56